Does :fiber cause weird side effects with embedded callbacks?

Does :fiber cause weird side effects with embedded callbacks?

It shouldn't. That's what most of the tests in this PR are designed to ensure: that nested fibers inherit state from their parent fiber. The entire point of this PR is to make sure Mongoid can be compatible with fiber-based libraries like Falcon.

Cool. 2 more:

1. Is it correct to assume that "fiber" isolation also implies "thread" isolation, as fibers exist on threads (and the "main fiber" of each thread is a separate fiber?) Are there tests for this?
2. Aside from not being on the latest Ruby, is there any reason that a user using Puma (not Falcon) wouldn't want to use :fiber isolation? If no (i.e. fiber is safe) then how about making it the default in Mongoid 10?

Awesome, thanks @jamis!

Only one comment from me, does it make sense to default the value of Mongoid.isolation_level to the value of ActiveSupport::IsolatedExecutionState.isolation_level? I believe ActiveRecord does, so for continuity, it kind of feels like maybe Mongoid should as well? This way I don't have to remember to set it twice.

P.S. Falcon also assumes this is the standard entry point to make such decisions: https://github.com/socketry/falcon/blob/main/lib/falcon/railtie.rb

@travisbell -- great suggestion. I'll look into it. It's a bit complicated by the fact that we still support Rails 6.x, and the IsolatedExecutionState wasn't introduced until sometime in 7.x, but it's a good idea.

@johnnyshields :

1. Is it correct to assume that "fiber" isolation also implies "thread" isolation, as fibers exist on threads (and the "main fiber" of each thread is a separate fiber?) Are there tests for this?

See https://github.com/mongodb/mongoid/pull/6009/files#diff-fc964a96de10aff651e8e1d4a35745a2a33f357819002b11af3cdacfe3e56979R58-R78

2. Aside from not being on the latest Ruby, is there any reason that a user using Puma (not Falcon) wouldn't want to use :fiber isolation? If no (i.e. fiber is safe) then how about making it the default in Mongoid 10?

It's worth considering. Again, "aside from not being on the latest Ruby" is the sticking point. As long as we support Ruby prior to 3.2, we really can't make :fiber the default.

@travisbell I've updated the PR so that Mongoid will default the isolation state to whatever it is set to in Rails; it had to take several edge cases into account, but it works now.

Would you be able to try this PR with your app, to see if it mitigates some of the problems you were seeing with Mongoid+Falcon? Otherwise, I'll see if I can unpack Falcon enough to set something up locally, myself.

@jamis I just left for vacation but when I get back (end of July), I’ll give it a spin and report back. 👍🏼

@jamis I've been running this branch in prod for the past day or so and everything is looking great. I haven't seen the previous issue with mis-matched queries so I believe this is looking good. With these changes I think we can officially say Falcon is supported! 💪🏼

@travisbell That's wonderful! Thanks for giving it a spin and letting me know. That gives me confidence to move forward with this.

Hey @jamis, turns out using Fiber[] still has a few problems. The primary being (I believe) that Async tasks (ie. fibers) can be nested and if multiple Mongoid queries are made, it can introduce the same issue as before--the current scope gets mangled with other queries because they share the same parent Fiber.

I currently have this commit running in production, which seems to properly solve it, simply using Fiber.current instead. However, you'll see I had to make some other changes in order to do this, which bring me to a broader question with this API design...

I think there's some opportunity to optimize these lookups a bit, for example, I don't think we need to continuously evaluate which isolation scope to use on every single storage lookup. We should be able to set the storage scope once during the config and persist it, shouldn't we? This assumes that during a single request/query lifecycle one cannot change the isolation scope. That seems like a valid assumption to me, but I don't know if that's something you think is needed.

Anywho, let me know what you think about a change like this.

@travisbell, thanks for the follow up. One of the problems with fibers in general, here, is that it is entirely possible to run fibers within the context of other fibers, which is not a situation that can occur with threads. Ruby's fiber-local storage allows fiber state to be inherited by these child fibers, which is (I believe) a feature we need. Otherwise, it is possible for Mongoid to "lose track of itself" in certain query configurations, when it spawns fibers itself to process callbacks on embedded children. (Those spawned fibers need to be able to see the Mongoid state that was created by the spawning fiber.)

I know concurrency issues are difficult to nail down, but is there any way you could provide a way for me to reproduce the issues you're seeing? I tried several things when I was developing this PR, but everything I tried was probably far too naive to actually tickle the bug.

Knowing how it's failing in our environment might make it a bit easier for me to create a reproduction. Let me take a stab at it...